cgirgen: remove the var expression fixup
#1482
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For the sake of making detection of ill-formed MIR code easier, an internalAssert is temporarily added to the procedure.
Arguments to `var` parameters weren't hoisted correctly, resulting in arguments of `var` type that are not `HiddenAddr` expressions.
zerbina
added
compiler/backend
There are two places where hook calls are manually constructed, both which didn't wrap the first argument in a `nkHiddenAddr` tree.
Method dispatcher creation didn't account for `var` parameters, placing them in argument positions directly.
It only has a single callsite and is now short and simple enough to no longer warrant being a procedure. The assertion is also removed again.
zerbina
changed the title
var argument fixupvar expression fixup
saem
approved these changes
Dec 26, 2024
| @@ -3491,10 +3491,22 @@ proc hoistParamsUsedInDefault(c: PContext, call, letSection, defExpr: var PNode) | |||
| newNodeI(nkEmpty, letSection.info), | |||
| call[paramPos]) | |||
|
|
|||
| call[paramPos] = newSymNode(hoistedVarSym) # Refer the original arg to its hoisted sym | |||
| if hoistedVarSym.typ.kind == tyVar: | |||
There was a problem hiding this comment.
we don't need to consider any skipTypes/unwrapping because this is after generic resolution etc, yes?
There was a problem hiding this comment.
In part, yes. There's also the assumption throughout parts of the compiler that tyVar cannot appear be nested in a tyAlias or the like.
Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
|
/merge |
|
Merge requested by: @saem Contents after the first section break of the PR description has been removed and preserved below:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Remove a fixup in
cgirgenrelated tovarparameters incgirgenand correct the AST construction necessitating it. There's no change in
language semantics.
Details
In typed
PNodeAST,varparameters always have to be dereferencedfirst, including when being passed to other
varparameters (resultingin
(HiddenAddr (HiddenDeref x))trees). Multiple transformations /places where AST is constructed in the compiler didn't produce AST
adhering to this.
The result was incorrect MIR code being generated (because the location
storing the
varparameter was being passed to another parameter,not the location the
varparameter references), though this didn'tcause any trouble so far.
cgirgendetected this situation andadjusted its output accordingly, producing well-formed CGIR AST.
The sources of the problematic
PNodeAST were:semexprsliftdestructorscgmethThey're changed to produce AST adhering to the aforementioned
requirements, and the fixup in
cgirgenis removed.